feat: remove dpr + ui default virtual screen + default screen inset area - #1489
Conversation
Deploying js-sdk-toolchain with
|
| Latest commit: |
e108df3
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://88f53e48.js-sdk-toolchain.pages.dev |
| Branch Preview URL: | https://chore-ui-default-vscreen-def.js-sdk-toolchain.pages.dev |
Test this pull request
|
… into chore/ui-default-vscreen-default-screen-inset
`uiScaleFactor` is now exactly the contain-fit of the design resolution inside the canvas, and `scaleOnDim` resolves 'Nvw'/'Nvh' and string `fontSize` to N% of the canvas dimension, as in CSS. devicePixelRatio is a density hint for picking a 1x/2x/3x asset, and each renderer computes it differently, so dividing by it made UI size inversely proportional to whichever value the scene happened to get. The field stays on PBUiCanvasInformation and `ScaleContext.ratio` stays in the public API. No renderer change is required. BREAKING CHANGE: scenes that set a virtual size and were calibrated against the current behaviour will render devicePixelRatio times larger. Scenes that pass no virtual size keep uiScaleFactor === 1 and are unaffected. (cherry picked from commit 2995d67)
Drawing under a notch, a status bar or a rounded corner is a bug in every scene that ships it, so the device safe area is now what a renderer gets when it doesn't ask for anything. Creators who do want the whole screen opt in with `screenInset: 'none'`, and `'interactable'` is still there for UI that must also clear the Explorer's native HUD. Both setUiRenderer and addUiRenderer default to it, each renderer still honoring its own value. Test suites that assert the UI tree hanging directly off the canvas root now pass the shared WHOLE_SCREEN option, so the wrapper entity stays covered in ui-renderer-screen-inset.spec.tsx and out of everyone else's assertions. BREAKING CHANGE: a scene that passes no screenInset now renders inside the device safe area instead of the whole screen, with one extra wrapper entity in the UI tree.
… into chore/ui-default-vscreen-default-screen-inset
decentraland-bot
left a comment
There was a problem hiding this comment.
Review Summary
PR: #1489 — chore: remove dpr + ui default virtual screen + default screen inset area
Branch: chore/ui-default-vscreen-default-screen-inset → main
Files changed: 20 (+1,044 −193)
Three well-motivated changes: removing devicePixelRatio from layout (it was a density hint, not a layout unit), adding default virtual screens (1920×1080 desktop / 1600×720 mobile), and adding per-renderer screenInset with a safe 'device' default. The architecture is clean — the injectable isMobile provider correctly solves the react-ecs ↛ ~system/Runtime dependency boundary, the symbol-based scale-factor ownership prevents cross-system conflicts, and the per-tick resolution is lightweight (one Map scan + arithmetic, guarded by a change check).
Test coverage is thorough: 572 new lines across virtual-size-defaults.spec.tsx and ui-renderer-screen-inset.spec.tsx, plus updates to 10 existing test files. Edge cases (NaN, zero, dpr=0, late mobile detection, re-wrapping across setUiRenderer calls) are all exercised.
API Surface & Consumer Impact
The TypeScript API changes are backward-compatible: virtualWidth/virtualHeight moved from required to optional, and the new screenInset field is optional. Existing callers compile without changes.
Behavioral changes that affect scenes without code changes:
- Scenes calling
setUiRenderer(ui)with no options now get a default virtual screen (previously: no scaling) - All renderers now wrap UI in the device safe area by default
- High-DPR screens will render UI at a different size (no longer divided by
devicePixelRatio)
The PR correctly argues for a minor bump (7.26.0) to signal these behavioral changes. Scenes already wrapping UI in <ScreenInsetArea> will get double-insetting — the PR documents the workaround (screenInset: 'none').
No consumers of UiRendererOptions, setUiRenderer, or addUiRenderer outside js-sdk-toolchain and sdk7-goerli-plaza would break at the TypeScript level. The sdk-skills repo references the old type signature and should be updated when this merges.
Security
No security issues found. No secrets, no injection surfaces, no sensitive data in logs. The setIsMobileProvider export is not reachable from the public @dcl/sdk/react-ecs surface; even if called by a scene, it would only affect that scene's own UI.
Findings
[P2] Stale comment in WHOLE_SCREEN JSDoc — test/react-ecs/utils.ts:9
The comment says "The default inset ('interactable')" but the actual default is 'device' (system.ts:75). Three independent reviewers flagged this. Easy fix: s/interactable/device/.
[P2] Providing only one virtual dimension silently disables scaling — system.ts:77-82
hasVirtualSize({ virtualWidth: 1920 }) returns true (blocks the platform default), but isValidVirtualSize returns false (virtualHeight is undefined → 0 → not > 0), so scaling is disabled entirely with no diagnostic. A one-time console.log warning — matching the mobile-override logging pattern already in place — would help creators catch this misconfiguration.
[P2] wrapWithScreenInset switch lacks exhaustiveness check — system.ts:209-217
The default branch handles 'none', so adding a fourth UiScreenInset variant in the future would silently fall through. An explicit case 'none': + default: { const _: never = resolvedInset; } is the standard TypeScript exhaustiveness pattern.
[P2] ScaleContext.ratio is now vestigial for layout — utils.ts:207-212
The field stays on ScaleContext as a density hint per the comment, and the new test explicitly asserts layout independence from ratio. If no caller currently reads it, consider @deprecated to signal this.
[P2] Deep dist/ import path — @dcl/sdk/src/react-ecs.ts:21
import { setIsMobileProvider } from '@dcl/react-ecs/dist/platform' couples to the build output layout. A dedicated exports subpath in package.json (e.g., @dcl/react-ecs/platform) would be cleaner, though dist/ imports may be an established monorepo convention.
Git Conventions (ADR-6)
- Branch name ✅
chore/<summary> - PR title ✅
<type>: <summary>— thoughfeat:might better reflect the behavioral additions (default virtual screen, screenInset option).choreper ADR-6 is "no production code change," and this PR clearly changes production behavior.
Verdict: ✅ APPROVE
No P0 or P1 issues. The P2 findings are minor improvements — the stale comment and the single-dimension edge case are the most actionable. The PR is well-designed, well-documented, and well-tested.
Reviewed by Jarvis 🤖 · Requested by Pravus (<@UDJQDQC0Z>) via Slack
Three related UI-renderer changes, all bound to the "Creators Success" signal that UI must be rebuilt/tested separately for desktop and mobile. The virtual-screen half is inherited from the reverted #1444.
1.
devicePixelRatioout of UI layoutThe UI scale factor is now the contain-fit of the design resolution inside the canvas, and nothing else:
scaleOnDimdrops it too, so1vwis 1% of the canvas width and1vh1% of its height, exactly as in CSS.devicePixelRatiostays onUiCanvasInformationand onScaleContextas an informational density hint — the "pick a 1x/2x/3x asset" signal — which is the same role it has in CSS and React Native, where it is exposed but never enters layout.2. Default virtual screen
How
setUiRenderer/addUiRendererresolve the virtual screen used for UI scaling:console.lognotifies the creator once per provided size, not every tick.The virtual size stays a single scene-wide value: main renderer options win, else the first additional renderer that passed dimensions, else the platform default. Options carrying no dimensions (e.g. only a
screenInset) are skipped, so they don't read as a provided-but-invalid size. Defaults apply to scenes that only ever calladdUiRenderer()too.Resolution runs every tick in
UiScaleSystem, so it reacts correctly when async platform detection resolves to mobile a few frames after scene start. The virtual screen only applies while a renderer is registered; with no UI at all the scale factor is released.3. Default screen inset area
Adds an optional
screenInsettoUiRendererOptionsselecting the screen area a renderer's UI is positioned in:'device'(default) → the device safe area (excludes notch, status bar, rounded corners), fromUiCanvasInformation.screenInsetArea. Zero on desktop, so a no-op there.'interactable'→ the area free of the Explorer's native HUD (minimap, chat, …), fromUiCanvasInformation.interactableArea.'none'→ the whole screen,0,0at its top-left corner; no wrapper entity is added.'device'is the default because drawing under a notch, a status bar or a rounded corner should be something a creator opts into, not the out-of-the-box behavior. Each renderer honors its own value, so the main UI and additional renderers can use different insets simultaneously.Inset values are reported by the renderer in canvas pixels, and raw pixel props are multiplied by the UI scale factor when parsed — so
ScreenInsetArea/InteractableAreanow pre-divide the insets by the scale factor (compensateInsetForUiScale), keeping the values sent to the renderer in canvas pixels at any virtual screen size.How
@dcl/react-ecscannot depend on~system/Runtime(and depending on@dcl/sdkwould be circular), so a newsrc/platform.tsexposes an injectableisMobileprovider defaulting to non-mobile.@dcl/sdk/react-ecswires that provider to@dcl/sdk's existingisMobile()helper at module load.system.tsresolves the active virtual size, applies the default / disable / mobile-override rules, and wraps each renderer's component in its selected inset area.API changes
Versioning
All three changes alter the rendered result of scene code that was not modified — a scene that passed no options gains a virtual screen, UI on mobile gains a device inset, and pixel sizes change on high-density screens. That argues for a minor bump (7.26.0) rather than the patch (
7.25.1) that oddish currently computes off the latest tag.Creator-facing note
A UI already wrapped in
<ScreenInsetArea>now sits inside a renderer that is also insetting by default, applying the margin twice. Either drop the wrapper or passscreenInset: 'none'.Tests
virtual-size-defaults.spec.tsx— default / invalid / mobile-override resolution, includingaddUiRenderer-only scenes.ui-renderer-screen-inset.spec.tsx— per-renderer inset wrapping.interactable-area.spec.tsx/screen-inset-area.spec.tsx— inset scale compensation.virtual-scale-array.spec.tsx,transform.spec.tsx,label.spec.tsx,tree.spec.tsx,background.spec.tsx,button.spec.tsx,add-ui-renderer.spec.tsx— updated for the dpr-free scale factor and the default inset wrapper.ui.ts.crdtsnapshot updated: now requires~system/Runtimefor platform detection, adds the inset wrapper entity, and the bundle grows 410.7k → 412.6k.